Skip to content
This repository has been archived by the owner on Sep 1, 2024. It is now read-only.

mapOf and setOf validators #289

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

conartist6
Copy link
Contributor

Fixes #190

I also intend to add iterableOf to this.

@conartist6
Copy link
Contributor Author

One of the things that I'm sure is worth discussing is the structure of the mapOf call, currently PropTypes.mapOf([keyValidator, valueValidator]). I thought it was appropriate to use an array, because this will be closely related to PropTypes.iterableOf. In fact quite likely I will delegate, making mapOf([k, v]) internally call iterableOf([k, v]). It feels internally consistent to me for these two validators to be related.

Copy link
Contributor

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, the tests are great!

It's not yet certain that this is something that will be added, and it's blocked on my comments below, but having the PR is helpful.

factoryWithTypeCheckers.js Outdated Show resolved Hide resolved
factoryWithTypeCheckers.js Outdated Show resolved Hide resolved
@ljharb ljharb added new validator request Requests for a new kind of validator. Semver: minor labels Aug 29, 2019
@conartist6
Copy link
Contributor Author

conartist6 commented Aug 30, 2019

So in order to allow validators to delegate to each other, e.g. mapOf delegating to iterableOf I'm going to need to do some further work with secret. Currently the design is that the validators themselves don't have the secret, so they can't possibly delegate. I assume the reason they don't have it is because that call site also would feed props into a custom validator, and the intent is to avoid leaking the secret that way.

EDIT: I'm dumb, the secret lives in a lib module. I can just use it.

@conartist6
Copy link
Contributor Author

I'm going to add a tupleOf validator as well since I need it to implement mapOf (partly) as iterableOf(tupleOf([keyType, valueType])).

@conartist6
Copy link
Contributor Author

@ljharb I guess I can't dismiss your review as stale, but I believe I have made the changes that you requested, including your improved method for detecting Map and Set instances.

Please don't ship this yet though. I have to finish writing the tests. I'm just waiting to do it until I have more of an idea whether you would accept these changes.

@conartist6
Copy link
Contributor Author

OK I take back what I said. All the tests are here. CHANGELOG and README are updated. When you get to looking at this, it is ready to merge from my perspective.

@@ -1,3 +1,6 @@
## 15.7.3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding a new thing is semver-minor

Suggested change
## 15.7.3
## 15.8.0

@@ -269,6 +288,107 @@ module.exports = function(isValidElement, throwOnDirectAccess) {
}
return createChainableTypeChecker(validate);
}

function createTupleOfTypeChecker(typeCheckerTuple) {
var argsInvalid = !Array.isArray(typeCheckerTuple) || !typeCheckerTuple.every(function (checker) { return typeof checker === 'function'});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var argsInvalid = !Array.isArray(typeCheckerTuple) || !typeCheckerTuple.every(function (checker) { return typeof checker === 'function'});
var argsInvalid = !Array.isArray(typeCheckerTuple) || typeCheckerTuple.some(function (checker) { return typeof checker !== 'function'});

return new PropTypeError('Property `' + propFullName + '` of component `' + componentName + '` has invalid PropType notation inside iterableOf.');
}
if (!(Symbol && Symbol.iterator)) {
return new PropTypeError('Cannot use `PropTypes.iterableOf` if Symbol.iterator is not present');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically this isn't true; you could use Array.from exclusively, which can be polyfilled.

var keyChecker = typeCheckers[0];
var valueChecker = typeCheckers[1];
if (typeof Map !== 'function') {
return new PropTypeError('Cannot use `PropTypes.mapOf` if the Map data structure is not present');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this necessary? if Map isn't present, isMap will fail.


function validate(props, propName, componentName, location, propFullName) {
if (typeof Set !== 'function') {
return new PropTypeError('Cannot use `PropTypes.setOf` if the Set data structure is not present');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@@ -0,0 +1,9 @@
module.exports = function forEach(iterable, callback) {
var iterator = iterable[Symbol.iterator]();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this utility should use Array.from, and fall back to explicit array/string iteration

@ljharb ljharb marked this pull request as draft December 22, 2021 21:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.set and .setOf (and .map and .mapOf)
3 participants